Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DiagID Control packet Handling #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

DiagID Control packet Handling #11

wants to merge 2 commits into from

Conversation

KyleDengChunkai
Copy link

Add support for DiagID V1, V2 and V3 Control Packet definitions and subsequent feature handling.

quic-clew
quic-clew suggested changes Nov 19, 2024
View reviewed changes
router/app_cmds.c Outdated Show resolved Hide resolved
router/app_cmds.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/app_cmds.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.h Outdated Show resolved Hide resolved
@KyleDengChunkai KyleDengChunkai force-pushed the master branch 3 times, most recently from 0a585df to ca8614f Compare November 25, 2024 09:24
quic-clew
quic-clew suggested changes Nov 26, 2024
View reviewed changes
router/diag_cntl.h Show resolved Hide resolved
router/diag_cntl.h Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.h Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
@quic-clew
Copy link

Also can you update the commit message to follow the standard format within the diag project? Commit 1 should be something like:

diag_cntl: Add support for diag id v2 and v3 control commands

<description of why its needed and how problem is solved>

signed-off-by<>

Second Commit should look like:

apps_cmds: Add support for get diag ID command

<description of what it is for>

signed-off-by<>

@KyleDengChunkai KyleDengChunkai force-pushed the master branch 2 times, most recently from 3df5570 to e6f5f30 Compare December 4, 2024 08:20
quic-bmanoj
quic-bmanoj reviewed Dec 18, 2024
View reviewed changes
router/diag_cntl.c Outdated Show resolved Hide resolved
quic-clew
quic-clew suggested changes Jan 2, 2025
View reviewed changes
router/app_cmds.c Outdated Show resolved Hide resolved
router/app_cmds.c Outdated Show resolved Hide resolved
router/app_cmds.c Outdated Show resolved Hide resolved
router/app_cmds.c Outdated Show resolved Hide resolved
router/app_cmds.c Outdated Show resolved Hide resolved
router/app_cmds.c Outdated Show resolved Hide resolved
router/app_cmds.c Show resolved Hide resolved
quic-clew
quic-clew reviewed Jan 3, 2025
View reviewed changes
router/diag.h Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.c Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
router/diag_cntl.c Outdated Show resolved Hide resolved
quic-clew
quic-clew reviewed Jan 3, 2025
View reviewed changes
router/diag.h Outdated Show resolved Hide resolved
router/diag.h Outdated Show resolved Hide resolved
@KyleDengChunkai KyleDengChunkai force-pushed the master branch 3 times, most recently from 116f8d0 to 8ea2c2d Compare January 7, 2025 08:04
@KyleDengChunkai
diag_cntl: Add support for diag id v2 and v3 control commands
e62f312 
Adding support for Diag-ID handshake and storing the information.
Diag-ID indicates and indexes the identity of the clients.

Signed-off-by: Kyle Deng <[email protected]>
@KyleDengChunkai
apps_cmds: Add support for get diag ID command
3b3bea7 
Adding support for the diag-id command response

Signed-off-by: Kyle Deng <[email protected]>
@Srinivas-Kandagatla
Copy link

Current state of the diag router is totally unusable from SM8450.
If these changes are intended to fix that, then its not working.
I have tried this on my XElite Platform and I get
diag-router: [lpass] unsupported control packet: 28 CNTL 0000: 1c 00 00 00 08 00 00 00 01 00 00 00 00 80 00 00 ................

quic-clew
quic-clew reviewed Feb 7, 2025
View reviewed changes
router/app_cmds.c
@@ -38,6 +38,7 @@
#include "dm.h"
#include "hdlc.h"
#include "util.h"
#include "diag_cntl.h"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these headers are in alphabetical order, move this under "diag.h"

router/app_cmds.c
struct diag_id_tbl_t *diag_id_item = NULL;
struct list_head *diag_ids_head = NULL;
uint8_t resp_buffer[DIAG_MAX_RSP_SIZE] = {0};
uint8_t *offset_resp = NULL;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offset_resp does not need to be null initialized, the first use of it is to assign it to resp_buffer

router/app_cmds.c
struct list_head *diag_ids_head = NULL;
uint8_t resp_buffer[DIAG_MAX_RSP_SIZE] = {0};
uint8_t *offset_resp = NULL;
uint8_t process_name_len = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't used anymore?

router/app_cmds.c
@@ -117,11 +121,56 @@ static int handle_keep_alive(struct diag_client *client, const void *buf,
return dm_send(client, resp, sizeof(resp));
}

static int handle_diag_id(struct diag_client *client, const void *buf, size_t len)
{
struct diag_cmd_diag_id_query_req_t {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diag_id_query_req

router/app_cmds.c
uint16_t subsys_cmd_code;
uint8_t version;
} __packed;
struct diag_cmd_diag_id_query_rsp_t {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diag_id_query_resp

@quic-clew
Copy link

Current state of the diag router is totally unusable from SM8450. If these changes are intended to fix that, then its not working. I have tried this on my XElite Platform and I get diag-router: [lpass] unsupported control packet: 28 CNTL 0000: 1c 00 00 00 08 00 00 00 01 00 00 00 00 80 00 00 ................

I don't think we support this control packet on our downstream branch either. Will check with Kyle to see if he tested this on XElite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quic-clew quic-clew quic-clew requested changes

@quic-bmanoj quic-bmanoj quic-bmanoj left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KyleDengChunkai @quic-clew @Srinivas-Kandagatla @quic-bmanoj